Skip to content

Conversation

@felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Jan 23, 2021

Description

As fixing an iPad layout issue in the old issues view proved too cumbersome, I've replaced the entire implementation with a new issue view, based on code already there and in use for cards and tables.

The result provides a more modern look, is consistent with the rest of the app and shrinks the code base by ~ 220 lines of code.

Screenshots (if appropriate):

Previously Certificate warning
Simulator Screen Shot - iPhone 8 - 2021-01-23 at 13 13 48
Previously Error
Simulator Screen Shot - iPhone 8 - 2021-01-23 at 13 16 45

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

QA

Checklist: #874 (comment)

Bugs & improvements:

- ConnectionIssueViewController:
	- compare against certificate stored in the bookmark where available
	- remove text-based rendering via CertificateViewController (and remove the class), replace it with the standard certificate view controller
- adaptions to new ThemeCertificateViewController initialization where needed
- update SDK
	- remove IssuesViewController, ConnectionIssueViewController, IssuesPresentationAnimator and IssuesDismissalAnimator
	- create modern replacement leveraging existing code: IssuesCardViewController
	- migrate code from ConnectionIssueViewController to IssuesCardViewController
	- add new helper function to compare DisplayIssues issue levels without having to use .rawValue
- MoreViewController
	- becomes FrameViewController
	- adds the ability to also add a footer view
	- cleanup: removal of unused properties and initializers
- StaticTableViewRow: add .messageStyle support
- ThemeTableViewCell: add .messageStyle support and add additional CellStyler block API based on also new CellStyleSet
- AlertView:
	- add support for .accessibilityIdentifier
	- add option for custom .contentPadding
	- add button-only mode
- CardPresentationController:
	- ensure .dismissable is actually respected
	- fix premature release of CardTransitionDelegate that could lead to normal presentation rather than as a card
@felix-schwarz felix-schwarz added this to the 11.5.0-Current milestone Jan 23, 2021
@CLAassistant
Copy link

CLAassistant commented Jan 23, 2021

CLA assistant check
All committers have signed the CLA.

@felix-schwarz felix-schwarz requested review from hosy and jesmrec January 23, 2021 11:57
@felix-schwarz felix-schwarz changed the base branch from feature/tls-diff to milestone/11.5 January 25, 2021 09:45
@jesmrec
Copy link
Contributor

jesmrec commented Jan 26, 2021

QA checks

iPhone

iPad

@jesmrec
Copy link
Contributor

jesmrec commented Jan 26, 2021

(1)

The Review Connection message when http connection is stablished in landscape

Screen Shot 2021-01-26 at 12 05 00

text does not fit correctly to the background yellow chart.

If portrait mode is set again after landscape:

Screen Shot 2021-01-26 at 12 05 05

iPhoneXR iOS14
iPadAir iOS13

- FrameViewController: include table background color pin theming support
- ThemeTableViewCell:
	- support recreation of labels and custom layout of these labels
	- add primaryTextLabel and primaryDetailTextLabel accessors to access text labels irrespective of whether they are system-provided or recreated
- StaticTableViewRow
	- add support for recreated labels and their custom layout
- IssuesCardViewController
	- replace usage of constraints in CardCellBackgroundView with auto resized views (fixes auto layout constraint warnings)
	- switch to using primaryTextLabel and primaryDetailTextLabel and a custom recreatedLabelLayout
@felix-schwarz
Copy link
Contributor Author

@jesmrec Thanks. I could reproduce this only when rotating the device. It boiled down to iOS not respecting the provided margins after rotating the table cell view. Fixed as of 83eeab1.

@jesmrec
Copy link
Contributor

jesmrec commented Feb 1, 2021

Totally fixed. Approved on my side

@jesmrec jesmrec added the Approved by QA Approved by QA label Feb 1, 2021
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@hosy hosy merged commit 7a7e825 into milestone/11.5 Feb 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/new-issue-view branch February 1, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved by QA Approved by QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants